Skip to content

Add support for directives on directive definitions#4521

Merged
yaacovCR merged 26 commits intographql:16.x.xfrom
BoD:directive-on-directive-definitions
Apr 24, 2026
Merged

Add support for directives on directive definitions#4521
yaacovCR merged 26 commits intographql:16.x.xfrom
BoD:directive-on-directive-definitions

Conversation

@BoD
Copy link
Copy Markdown
Contributor

@BoD BoD commented Jan 6, 2026

Allow directives on directive definitions, based on this spec PR which introduces this syntax:

directive @onDirective on DIRECTIVE_DEFINITION

directive @foo @onDirective on OBJECT

directive @baz @deprecated(reason: "...") on OBJECT

extend directive @quux @deprecated(reason: "...")

Disclaimer

First time on this codebase and am also not a JS/TS person, so obvious mistakes and/or missing pieces are very likely 😅 Any help and feedback to improve this PR are very welcome 🙏. The tests seem to pass but additional tests may be needed.

@BoD BoD requested a review from a team as a code owner January 6, 2026 17:46
@vercel
Copy link
Copy Markdown

vercel Bot commented Jan 6, 2026

@BoD is attempting to deploy a commit to the The GraphQL Foundation Team on Vercel.

A member of the Team first needs to authorize it.

This was referenced Jan 6, 2026
Merged
Open
martinbonnin
martinbonnin reviewed Mar 17, 2026
View reviewed changes
Comment thread src/language/parser.ts Outdated
Comment thread src/language/parser.ts Outdated
This was referenced Mar 17, 2026
Merged
Open
@leebyron
Copy link
Copy Markdown
Contributor

leebyron commented Apr 2, 2026

Update includeDeprecated arg per @benjie 's comments, then we'll merge

@BoD
Copy link
Copy Markdown
Contributor Author

BoD commented Apr 3, 2026

Just made the change for includeDeprecated: Boolean! + added one test.

jerelmiller
jerelmiller reviewed Apr 3, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a few suggestions, mostly around types, but otherwise looks good from my end!

You probably want to get a proper review from someone who knows the conventions in this codebase better than I do, but wanted to try and help move this along in some way!

Comment thread src/language/parser.ts Outdated
Comment thread src/language/parser.ts Outdated
Comment thread src/utilities/__tests__/extendSchema-test.ts Outdated
Comment thread src/type/directives.ts
Comment thread src/utilities/extendSchema.ts Outdated
Comment thread src/utilities/extendSchema.ts Outdated
Comment thread src/utilities/extendSchema.ts Outdated
Comment thread src/utilities/extendSchema.ts Outdated
Comment thread src/utilities/extendSchema.ts Outdated
@benjie
Copy link
Copy Markdown
Member

benjie commented Apr 7, 2026

I've not fully reviewed this PR, but the changes since Lee's review all look fine to me - thanks @BoD! 👍

BoD and others added 18 commits April 24, 2026 12:41
@BoD @yaacovCR
Add support for directives on directive definitions
531cbf4
@BoD @yaacovCR
Introspection related changes
e46ed2e
@martinbonnin @yaacovCR
Add ParseOption.experimentalDirectivesOnDirectiveDefinition
d44ff03
@martinbonnin @yaacovCR
npm run prettier
ca49ec5
@martinbonnin @yaacovCR
fix test coverage
ac59fc1
@martinbonnin @yaacovCR
fix imports
46a73d4
@martinbonnin @yaacovCR
try to fix coverage
3d0a91c
@martinbonnin @yaacovCR
try to make test coverage pass
8db915f
@martinbonnin @yaacovCR
make prettier happy
64a5698
@BoD @yaacovCR
Introspection: change includeDeprecated: Boolean to Boolean!
e91532e
@BoD @yaacovCR
Fix indent
c2dbb78
@BoD @yaacovCR
Add an introspection test for multiple directives with arguments appl…
a15d81b 
…ied to a directive
@BoD @jerelmiller @yaacovCR
Update src/language/parser.ts
e34401a 
Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
@BoD @jerelmiller @yaacovCR
Update src/language/parser.ts
9329e48 
Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
@BoD @jerelmiller @yaacovCR
Update src/utilities/__tests__/extendSchema-test.ts
30f7d58 
Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
@BoD @jerelmiller @yaacovCR
Update src/utilities/extendSchema.ts
edcc324 
Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
@BoD @jerelmiller @yaacovCR
Update src/utilities/extendSchema.ts
788f090 
Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
@BoD @yaacovCR
Apply a few minor fixes suggested by Jerel
e230a8c
@yaacovCR yaacovCR force-pushed the directive-on-directive-definitions branch 2 times, most recently from cd3b99e to 06a336c Compare April 24, 2026 09:46
@yaacovCR yaacovCR added spec RFC Implementation of a proposed change to the GraphQL specification PR: feature 🚀 requires increase of "minor" version number labels Apr 24, 2026
@yaacovCR yaacovCR force-pushed the directive-on-directive-definitions branch from 06a336c to a9e07a6 Compare April 24, 2026 09:52
@yaacovCR
Copy link
Copy Markdown
Contributor

yaacovCR commented Apr 24, 2026

@BoD dropped in a few commits with fixes, possibly spec for some of them has to be correlated => have to check out ci failures a bit later….

@yaacovCR yaacovCR force-pushed the directive-on-directive-definitions branch from a9e07a6 to e287ba1 Compare April 24, 2026 14:47
@yaacovCR yaacovCR merged commit ad9c519 into graphql:16.x.x Apr 24, 2026
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martinbonnin martinbonnin martinbonnin left review comments

@leebyron leebyron leebyron approved these changes

@yaacovCR yaacovCR yaacovCR approved these changes

+1 more reviewer

@jerelmiller jerelmiller jerelmiller left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

PR: feature 🚀 requires increase of "minor" version number spec RFC Implementation of a proposed change to the GraphQL specification

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@BoD @leebyron @benjie @yaacovCR @jerelmiller @martinbonnin